Repository navigation
feat(browser): server-side traced, /server and Next.js entries - #1128
JeremyFunk wants to merge 4 commits into
Conversation
- traced spans through the global tracer on the server (was a no-op), so Server Components and SSR loaders nest under the server's own spans - @maple-dev/browser/server: traced and serverTiming(), @opentelemetry/api only - @maple-dev/browser/nextjs: onRouterTransitionStart, MapleNavigation, reportNextError; /nextjs/server: withMapleProxy - errors are recorded once per trace instead of once per process
- dedupe errors once per object again: per trace re-recorded errors that crossed an await in the browser, and raced across concurrent requests - withMapleProxy: build next() through the public API when there is no user proxy; tell the browser on a page load whose traceparent a load balancer added - MapleNavigation: key the effect on the route string, so a same-route commit (router.refresh, server action) doesn't end a navigation in flight - routeTemplate: match partly encoded segments by decoding them
Maple reviewConfidence 4/5 · likely safe to merge Adds server-side
What was checked
Observability coverage: 4 of 4 changes observable
|
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 1 remain after this review. 📝 WalkthroughWalkthroughThe browser package adds server-side tracing APIs and Next.js App Router integrations for navigation spans, error reporting, and trace propagation between proxy requests and server renders. Package exports, tests, and documentation are updated for these additions. ChangesBrowser package tracing integrations
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Feature Sequence Diagram(s)sequenceDiagram
participant Browser
participant withMapleProxy
participant Proxy
participant AppRender
Browser->>withMapleProxy: send request
withMapleProxy->>Proxy: invoke proxy
Proxy-->>withMapleProxy: return proxy response
withMapleProxy->>AppRender: forward trace context for app render
AppRender-->>withMapleProxy: return rendered response
withMapleProxy-->>Browser: return response with Server-Timing
Merge Risk: ⚪ Minimal · up to No actionable issue is established; the change is mergeable after normal checks. Architecture SummaryArchitecture risk: 🟡 Medium · up to The change affects 3 systems. Changed systems: Architecture concerns Review detailsSystems and components
Before / after behavior
Reliability and maintainability
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| const carried = request.headers.has("traceparent") | ||
| if (carried && request.headers.get("sec-fetch-dest") !== "document") return response | ||
| const result = | ||
| response ?? NextResponse.next(carried ? undefined : withTraceparent(request, traceparent)) | ||
| if (response && !carried) forwardToRender(response, request, traceparent) |
There was a problem hiding this comment.
🟡 Document render loses its incoming trace
When a proxy override drops an incoming traceparent, withMapleProxy treats the original request header as preserved. The render loses its parent while Server-Timing still joins the browser to the middleware trace.
Learn more
Next.js request-header overrides replace the headers passed to the render. A wrapped proxy can return NextResponse.next({ request: { headers } }) with a filtered header set. Here, carried checks the original request, not the override that the render receives. When that override omits traceparent, the wrapper skips forwarding it but still appends the middleware span to Server-Timing for a document request. The browser joins the middleware trace, while the render does not.
Example: A document request carries traceparent: 00-.... A tenant proxy forwards only x-tenant through NextResponse.next({ request: { headers: new Headers({ 'x-tenant': 'acme' }) } }). The HTML response advertises the middleware trace, but the render sees no traceparent.
Recommended fix: Determine whether the effective request headers delivered by NextResponse.next or a same-origin rewrite retain traceparent. Add the original value to the override when absent, without replacing an existing upstream value; preserve the proxy's other overridden headers.
Was this helpful? React with 👍 or 👎 to provide feedback.
| // An optional catch-all without segments has nothing to replace | ||
| if (!value?.length) continue | ||
| const parts: readonly string[] = typeof value === "string" ? [value] : value | ||
| const matchesAt = (at: number) => | ||
| parts.every((part, i) => segments[at + i] === part || decode(segments[at + i]) === part) | ||
| let at = end - parts.length | ||
| while (at > 0 && !matchesAt(at)) at-- | ||
| // Index 0 is the empty segment before the leading `/` | ||
| if (at <= 0) continue | ||
| segments.splice(at, parts.length, typeof value === "string" ? `[${name}]` : `[...${name}]`) |
There was a problem hiding this comment.
🟡 Optional catch-all route spans split
For /docs/[[...slug]], routeTemplate names /docs as /docs and /docs/a as /docs/[...slug]. The same route splits across span names, so route-level traces cannot group together.
Learn more
Next.js optional catch-all routes match both the base path and paths with one or more additional segments. useParams() supplies no slug for the base path, so skipping empty values leaves the base URL as the span name. For populated values, the current replacement uses the required catch-all form instead of the optional form. useParams() alone does not distinguish required and optional catch-all definitions when values are present.
Example: Visiting /docs under app/docs/[[...slug]]/page.tsx records pageload /docs; visiting /docs/install records navigate /docs/[...slug]. Both pages matched /docs/[[...slug]].
Recommended fix: Obtain route-pattern metadata from the Next.js route configuration or an integration-provided mapping if exact optional catch-all templates are required; otherwise document and normalize the grouping limitation explicitly rather than presenting both names as the matched route template.
Was this helpful? React with 👍 or 👎 to provide feedback.
| export function reportNextError(error: unknown): void { | ||
| // In production a Server Component's error reaches the browser with its | ||
| // message stripped and a `digest` added: one meaningless issue for all of them | ||
| if (typeof error === "object" && error !== null && "digest" in error && error.digest) return | ||
| captureException(error, { name: "react.render_error" }) |
There was a problem hiding this comment.
🟡 Global errors leave navigation spans open
When global-error.tsx replaces the root layout, MapleNavigation cannot finish the pending navigation. reportNextError records the error but leaves that span open until another navigation or page exit.
Learn more
The root layout normally renders MapleNavigation, whose effect ends an open navigation after the new route commits. Next.js renders global-error.tsx instead of the root layout for a root-level failure, so that effect does not run. The documented global error boundary calls reportNextError, which records the exception but never closes the navigation. The open span continues to parent later traced work until another navigation or pagehide interrupts it.
Example: A click to /settings starts a navigate span, then the root layout throws and global-error.tsx renders. Reporting that error records react.render_error, but the /settings navigation remains open while the error UI is shown.
Recommended fix: Have the global error integration explicitly close or interrupt the pending navigation when it reports the error, while avoiding changes to ordinary error.tsx handling where the layout and its navigation component can still commit.
Was this helpful? React with 👍 or 👎 to provide feedback.
…s drop the incoming one
Maple reviewConfidence 3/5 · needs attention Warning This review ended early; what follows is what it established. Adds server-side trace joining to the browser SDK: a shared
What was checked
Observability coverage: 2 of 2 changes observable
|
This branch has not been deployed
1 participant
Summary
Server-side helpers and a Next.js integration for
@maple-dev/browser, replacing the glue the frontend guides had customers copy.Fix:
tracedon the server.MapleBrowser.tracedwas a no-op withoutwindow, but guides use it for server-side data loading (Server Components, SSR loaders, SSR resolvers). On the server it now runsfnin a span from the global tracer, under the active context, so it nests under whatever the app registered (@vercel/otel, NodeSDK) and keeps its parent acrossawait. It doesn't touch navigation state. Without server OTel it only runsfn.New entries (the main entry's bundle is unchanged apart from the server
tracedbranch: eager 37.39 → 37.42 kB, first-party 14.25 → 14.30 kB, budgets untouched):@maple-dev/browser/server→traced(name, fn, options?)@maple-dev/browser/server→serverTiming(): string | undefinedtraceparent;desc="00-…"for the active span (sampled flag kept), for any framework's response hook@maple-dev/browser/nextjs→onRouterTransitionStart(url)instrumentation-client.ts: starts a span per App Router navigation@maple-dev/browser/nextjs→MapleNavigation"use client"component for the root layout; wraps its own<Suspense>, ends the span named after the route (/projects/[id],/[...slug],/_not-found)@maple-dev/browser/nextjs→reportNextError(error)error.tsx/global-error.tsx: skipsdigesterrors, dedupes@maple-dev/browser/nextjs/server→withMapleProxy(proxy?)proxy.ts/middleware.ts: requesttraceparent+ responseServer-Timing, composes with your proxy/serverand/nextjs/serverdepend only on@opentelemetry/api(+next/server), so they're safe in Node, edge runtimes and Workers.nextandreactare optional peers (*, so no existing install gets a peer conflict).Behaviour notes
traced,captureExceptionand the global handlers, on the server too. Scoping it per trace was tried and dropped: the browser loses the trace at everyawait, so an error re-entering from another trace was recorded twice, and concurrent requests raced on it. The trade-off: an error object shared by requests (a memoized promise's rejection) is recorded bytracedon the first request only. The framework's own spans still record the rest.withMapleProxyonly changes responses that go on to a render in this app (next(), same-originrewrite()). Redirects, your own responses (including immutable ones) and external rewrites pass through unchanged. With no user proxy it usesNextResponse.next({ request: { headers } }). Composing with a user'snext()/rewrite()extends the same override headers the way Next.js extends them for its router headers. Atraceparentthe render already receives is kept (a user'snext({ request })that drops it gets ours instead). Only a document request (Sec-Fetch-Dest: document, e.g. a load balancer's traceparent) still getsServer-Timing, so client-navigation RSC responses stay untouched.MapleNavigationkeys its effect on pathname, query and route template, not theuseParams()object, sorouter.refresh()or a revalidating server action doesn't end a navigation in flight. A link back to the route on screen ends an in-flight navigation as interrupted.Verification
packages/browser: typecheck, 143 tests (node + Chromium, including a React test forMapleNavigationand proxy tests against the realnext/server), build ("use client"stays at the top ofdist/nextjs.mjs), size budget.next build && next start), then driven with Playwright across 21 scenarios. Browser spans match the old run: pageload/navigate naming,/_not-found, interrupted, redirect, back/forward, query/hash, digest skip, render/loader error counts, propagation. The page load joinsmiddleware GET→ render → servertracedspan → serverfetchafter anawait→ API.MapleBrowser.tracedfrom the main entry in a Server Component nests underRSC GET /slow. One difference: a<Link>to an unmatched URL (full reload) now exports the old document'snavigatespan as interrupted instead of dropping it. That's the SDK's existingpagehidehandling, not new here.traceparentheader equals theServer-Timingone for no proxy, a usernext({ request }), a plainnext()and a same-originrewrite(). User request headers are kept. Redirects are untouched.Known limits
basePath: Next.js passes back/forward URLs with the basePath but push URLs without it. A back/forward to a hash entry can open a span that ends as interrupted./users/settings/settingsfor/users/[name]/settings) is ambiguous without the route tree.useParams()can't tell[[...slug]]from[...slug], so/docsisnavigate /docsand/docs/aisnavigate /docs/[...slug].global-error.tsxunmountsMapleNavigation, so that navigation ends as interrupted at the next navigation or when the page is left.Summary by CodeRabbit